fix #3845 #3847
fix #3845 #3847
Conversation
Summary of ChangesHello @guillaumeblaquiere, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical security concern by implementing a robust system to prevent the leakage of client secrets. It centralizes the secure, in-memory storage of these secrets, redacts them from configuration objects, and only exposes them transiently during necessary authentication flows, significantly enhancing the overall security posture of the system's authentication handling. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
adk-bot
added
the
core
|
Response from ADK Triaging Agent Hello @guillaumeblaquiere, thank you for your contribution! To help the reviewers better understand and verify this important security fix, could you please provide logs or a screenshot demonstrating that the This will help speed up the review process. Thanks! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to prevent a client secret leak by redacting it and storing it in memory. While the approach is generally correct, the implementation introduces a critical security vulnerability where a secret could be left un-redacted if an error occurs during an OAuth token exchange. Additionally, there's a high-severity issue where sensitive access tokens are leaked to stderr for debugging. I've also included several medium-severity comments to improve code quality, efficiency, and maintainability. It is crucial to address the security flaws before merging.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a critical security vulnerability by preventing client secrets from being leaked. The approach of redacting the secret and storing it in memory on the server-side is sound. The changes are extensive and include new logic in CredentialManager and AuthHandler, as well as new tests to cover the secret handling.
My review focuses on ensuring the new logic is correct, robust, and maintainable. I've identified a critical bug in the secret re-redaction logic, some code duplication that should be addressed, and issues in the new tests that could lead to flakiness. Please see my detailed comments below.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to fix a critical security issue where the client_secret was being leaked. The approach of redacting the secret upon CredentialManager initialization and using a context manager (restore_client_secret) to temporarily restore it for API calls is excellent. The addition of new unit tests to cover this new secret handling logic is also a great improvement. However, I've identified a critical security vulnerability in the new fallback logic within the _exchange_credential method. This logic could lead to using a secret for one client with another client's ID, which must be fixed. My review includes a specific comment with a suggested fix for this issue.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Hi @guillaumeblaquiere , Thank you for your work on this pull request. We appreciate the effort you've invested. Can you please fix the failing unit tests. |
ryanaiagent
added
the
request clarification
|
@ryanaiagent done. |
ryanaiagent
removed
the
request clarification
ryanaiagent
added
the
needs review
|
@guillaumeblaquiere ,Your PR has been received by the team and is currently under review. We will provide feedback as soon as we have an update to share. |
|
Hi @seanzhou1023 , can you please review this. |
ryanaiagent
added
request clarification
|
Hi @guillaumeblaquiere , we appreciate your patience and support. Can you please fix the failing unit tests and formatting errors. |
|
@ryanaiagent unit test fixed |
|
Hi @guillaumeblaquiere , pls fix the formatting error as well. |
|
@ryanaiagent pyink ok |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust client secret management mechanism by redacting client secrets from AuthConfig objects and storing them securely in memory within the CredentialManager. A context manager, restore_client_secret, is implemented to temporarily expose these secrets for operations like token exchange and URI generation, ensuring they are re-redacted afterwards. New unit tests have been added to validate this security enhancement and the stability of credential keys. Additionally, minor import cleanups and updates to test mocks have been performed across several files.
| return exchange_result.credential | ||
|
|
||
| # Restore secret if needed | ||
| credential = self.auth_config.exchanged_auth_credential |
There was a problem hiding this comment.
The credential object here is self.auth_config.exchanged_auth_credential. If the AuthHandler instance is initialized with the original auth_config object, and CredentialManager later deep copies this auth_config in its __init__ method, then self.auth_config.exchanged_auth_credential in AuthHandler will still refer to the original credential object, not the deep-copied one managed by CredentialManager. Consequently, calling CredentialManager.restore_client_secret(credential) might not operate on the correct, managed credential object, potentially bypassing the intended secret management or leading to unexpected behavior. Consider ensuring that AuthHandler always operates on the auth_config object that CredentialManager is actively managing, or retrieve the managed credential from CredentialManager directly.
There was a problem hiding this comment.
Can you address this.
| ) | ||
| client_id = auth_credential.oauth2.client_id | ||
|
|
||
| with CredentialManager.restore_client_secret(auth_credential): |
There was a problem hiding this comment.
Similar to the exchange_auth_token method, auth_credential here refers to self.auth_config.raw_auth_credential. If AuthHandler is initialized with the original auth_config and CredentialManager deep copies it, this auth_credential might not be the one managed by CredentialManager. This could lead to CredentialManager.restore_client_secret not restoring the secret to the correct object, potentially exposing the secret or causing functional issues. Ensure AuthHandler uses the auth_config object that CredentialManager is managing, or retrieve the managed credential from CredentialManager directly.
| def create_auth_config_mock(): | ||
| """Creates a mock AuthConfig that returns itself on model_copy.""" | ||
| # We remove spec=AuthConfig because accessing Pydantic fields on a spec-ed mock | ||
| # can fail if they are not seen as class attributes or if we need dynamic attributes. | ||
| m = Mock() | ||
| m.spec = AuthConfig # Optional: if we want isinstance to work, but Mock(spec=X) enforces attributes. | ||
| # Let's just use a plain Mock and configure what we need. | ||
| m.model_copy.side_effect = lambda **kwargs: m | ||
| return m |
There was a problem hiding this comment.
The create_auth_config_mock function's model_copy.side_effect = lambda **kwargs: m causes the mock to return itself when model_copy is called. This contradicts the behavior of pydantic.BaseModel.model_copy(deep=True), which is designed to create a new, independent copy of the object. In CredentialManager.__init__, auth_config.model_copy(deep=True) is called specifically to create an independent copy that can be modified (e.g., redacting secrets) without affecting the original object passed by the user. If the mock returns itself, _secure_client_secret will modify the original auth_config mock, which is not the intended behavior of model_copy and could lead to false positives in tests or mask actual bugs where the original auth_config is unexpectedly mutated. The test test_credential_manager_redacts_secrets_in_raw_credential in test_credential_manager_secrets.py correctly asserts that the original auth_config is not modified, highlighting this discrepancy. Consider returning a new, independent mock for model_copy to accurately simulate the deep copy behavior.
|
Hi @guillaumeblaquiere , can you please address the suggestions. |
|
@ryanaiagent I'm travelling the next 2 days. I will fix this then. |
4 participants
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Problem:
The client_secret was leaked and shared with the client
Solution:
I was unable to solve the issue myself. I vibe coded it with Antigravity.
I checked the code, it looks good to me.
Testing Plan
Unit Tests:
Manual End-to-End (E2E) Tests:
Manual test is OK